Skip to content

fix: Harden catalog skill installation validation and update tests#326

Merged
yacosta738 merged 4 commits intomainfrom
fix/catalog-installability-audit
Apr 6, 2026
Merged

fix: Harden catalog skill installation validation and update tests#326
yacosta738 merged 4 commits intomainfrom
fix/catalog-installability-audit

Conversation

@yacosta738
Copy link
Copy Markdown
Contributor

This pull request introduces a comprehensive set of improvements to the skill catalog system, focusing on enhanced catalog-driven skill resolution, support for legacy skill IDs, and improved skill suggestion accuracy. It also adds a new end-to-end (E2E) GitHub Actions workflow to validate the catalog, and updates documentation accordingly.

Catalog-driven skill resolution and installation:

  • The EmbeddedSkillCatalog is now used as the primary source for resolving skill install sources and archive subpaths, both in the CLI and in the SkillsShProvider, allowing for deterministic and overrideable skill installation. [1] [2] [3]
  • The catalog and provider data models (CatalogSkillDefinition, ProviderCatalogSkill, etc.) now support new fields: archive_subpath, legacy_local_skill_ids, and install_source, enabling more flexible and accurate skill resolution. [1] [2] [3] [4] [5]

Legacy skill ID support and improved suggestion logic:

  • Skill suggestions now track and match against legacy_local_skill_ids, ensuring that skills installed under previous IDs are recognized and not re-suggested. [1] [2] [3] [4] [5] [6] [7] [8]

Catalog E2E validation workflow:

  • Adds a new GitHub Actions workflow (.github/workflows/catalog-e2e.yml) that runs a scheduled and manual E2E test to ensure all catalog skills can be resolved, installed, and registered.
  • Updates the README.md to document the new workflow and provide instructions for running the E2E validation locally or via GitHub Actions. [1] [2]

Skill provider improvements:

  • The logic for constructing skill archive subpaths in SkillsShProvider now consults the embedded catalog for overrides and supports a wider range of skill repository naming conventions. [1] [2]

Other improvements:

  • Introduces a utility function to infer the install source format (tar.gz, zip, or dir) based on the URL, improving installation reliability.

These changes collectively improve the reliability, maintainability, and user experience of the skill catalog and installation system.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 6, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: f786636f-5be2-49e7-9f6b-f700b6c77678

📥 Commits

Reviewing files that changed from the base of the PR and between fffb85b and 0211efe.

📒 Files selected for processing (7)
  • .github/workflows/catalog-e2e.yml
  • README.md
  • src/commands/skill.rs
  • src/skills/catalog.rs
  • src/skills/catalog.v1.toml
  • src/skills/provider.rs
  • tests/test_catalog_integration.rs

📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Embedded skill catalog used for resolving and installing skills; install-source handling improved.
    • Legacy skill aliases supported so recommendations and installs recognize older skill IDs.
    • New "Catalog E2E" workflow to run end-to-end catalog validation.
  • Documentation

    • Added "Catalog validation" section to README with workflow badge and run instructions.
  • Tests

    • Expanded integration and unit tests covering catalog installs, install-source resolution, and legacy-alias behavior.

Walkthrough

Adds embedded-catalog fields and lookup paths (install_source, archive_subpath, legacy_local_skill_ids), updates provider and install-source resolution to prefer embedded catalog entries, extends suggestion/installed-state logic to consider legacy aliases, introduces E2E catalog workflow, and converts integration test to iterate embedded catalog entries.

Changes

Cohort / File(s) Summary
CI & Documentation
\.github/workflows/catalog-e2e.yml, README.md
Add scheduled/manual Catalog E2E workflow and README section describing how to run the catalog E2E test locally and via workflow.
Catalog Schema & Data
src/skills/catalog.rs, src/skills/catalog.v1.toml
Extend catalog entries with install_source, archive_subpath, legacy_local_skill_ids; add lookup APIs (get_archive_subpath, get_install_source, get_skill_definition_by_local_id); update TOML entries (many adds/removals, Vue canonical/legacy change).
Provider logic
src/skills/provider.rs
Add resolve_catalog_install_source and new provider constants; consult embedded catalog for archive_subpath when building archive URLs; adjust skills-subdirectory heuristics and URL fragment handling.
Commands & Resolution
src/commands/skill.rs
Prioritize embedded catalog in SuggestInstallProvider::resolve and resolve_source; add infer_install_source_format to map download URLs to formats.
Suggestion/Installed-state
src/skills/suggest.rs
Add legacy_local_skill_ids to SkillSuggestion, builder method, and installed_state_for_recommendation helper; use legacy aliases when checking installed state and when deciding to skip installs.
Integration & Unit Tests
tests/test_catalog_integration.rs, tests/integration/skill_suggest.rs, tests/unit/*.rs
Switch integration test to iterate embedded catalog and use blocking_fetch_and_install_skill with retry and registry checks; update unit/integration test fixtures to populate new catalog/provider fields and add tests for URL fragment behavior and legacy-alias install skipping.

Sequence Diagram

sequenceDiagram
    participant Client
    participant SuggestionService
    participant EmbeddedCatalog
    participant Provider
    participant Installer
    participant Registry

    Client->>SuggestionService: request recommendation/install for skill
    SuggestionService->>EmbeddedCatalog: lookup skill definition by provider/local id
    alt definition found with install_source
        EmbeddedCatalog-->>SuggestionService: return install_source / archive_subpath / legacy IDs
    else no catalog install_source
        SuggestionService->>Provider: resolve download URL / install source
        Provider-->>SuggestionService: return download URL
    end
    SuggestionService->>Registry: check installed state (canonical + legacy IDs)
    alt already installed
        SuggestionService-->>Client: skip install (AlreadyInstalled)
    else not installed
        SuggestionService->>Installer: fetch & install using resolved source
        Installer-->>Registry: record installed skill
        Installer-->>SuggestionService: success/failure
        SuggestionService-->>Client: report result
    end
Loading

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐰 I hop through catalogs, nose in code,
Old names remembered down the road,
Embedded maps and archive trails,
I fetch the bits and wag my tails,
Success—then nibble on a carrot node! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 41.86% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main focus of the PR: hardening catalog skill installation validation and updating tests. It directly reflects the primary changes in the changeset.
Description check ✅ Passed The description is comprehensive and closely related to the changeset, covering all major areas including catalog-driven resolution, legacy ID support, E2E workflow, and provider improvements.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/catalog-installability-audit

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sentry
Copy link
Copy Markdown

sentry bot commented Apr 6, 2026

Codecov Report

❌ Patch coverage is 67.70186% with 52 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/commands/skill.rs 43.90% 23 Missing ⚠️
src/skills/provider.rs 70.49% 18 Missing ⚠️
src/skills/catalog.rs 69.44% 11 Missing ⚠️

📢 Thoughts on this report? Let us know!

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.github/workflows/catalog-e2e.yml:
- Around line 49-53: Add a pre-check in the "Run catalog installation E2E" step
to fail fast if the vendored skills repo path in AGENTSYNC_LOCAL_SKILLS_REPO is
missing: before running cargo test, check that the directory referenced by the
environment variable AGENTSYNC_LOCAL_SKILLS_REPO exists (e.g., test -d
"$AGENTSYNC_LOCAL_SKILLS_REPO") and output a clear error and exit non-zero if it
does not; update the step that currently runs cargo test to run the directory
existence check first and only proceed to cargo test --test
test_catalog_integration -- --ignored --nocapture when the check passes so the
workflow fails fast on missing vendor checkout.

In `@README.md`:
- Around line 62-66: The README's "Local run" command omits the prerequisite of
having the canonical skills repo available; update the instruction to require
cloning or pointing to a local agents-skills checkout by setting
AGENTSYNC_LOCAL_SKILLS_REPO (or placing a sibling agents-skills checkout) before
running the tests referenced in tests/test_catalog_integration.rs, e.g. instruct
users to git clone the agents-skills repo or export AGENTSYNC_LOCAL_SKILLS_REPO
to the clone path and then run the existing RUN_E2E=1 cargo test command so the
test_catalog_integration can find the canonical repo.

In `@src/commands/skill.rs`:
- Around line 1082-1092: The function infer_install_source_format currently
assumes any HTTP(S) URL that isn't .tar.gz/.tgz is a "zip", which can be
misleading; update infer_install_source_format to explicitly check for ".zip"
(and treat it as "zip"), then for other HTTP(S) URLs return a clearer sentinel
such as "url" or "unknown" instead of always "zip", so callers can distinguish
explicit zip archives from other remote URLs; keep the existing "dir" return for
non-HTTP sources.
- Around line 928-934: The code constructs SkillInstallInfo with a computed
format from infer_install_source_format in the EmbeddedSkillCatalog branch but
that format value is never used by the install pipeline (see
blocking_fetch_and_install_skill and fetch_and_unpack_to_tempdir), so either
remove the dead computation or document its intent: delete the format field
population in the EmbeddedSkillCatalog path (remove the
infer_install_source_format call) if unused, or add a short comment next to
SkillInstallInfo construction (and/or on the SkillInstallInfo type) explaining
the format is informational/reserved for future use; update references to
EmbeddedSkillCatalog, get_install_source, SkillInstallInfo, and
infer_install_source_format accordingly.

In `@src/skills/catalog.rs`:
- Around line 237-244: The method get_skill_definition_by_local_id currently
does a linear scan over self.skill_definitions.values(), which can be slow at
scale; add a reverse index to map local_skill_id to the full definition or to
the provider_skill_id and then do an O(log N)/O(1) lookup. Concretely: add a new
field (e.g. local_to_provider: BTreeMap<String, String> or local_to_definition:
BTreeMap<String, CatalogSkillDefinition>) to the catalog struct, populate and
maintain it wherever skills are inserted/updated/removed (the same code paths
that touch skill_definitions and local_skills/CatalogSkillMetadata), and change
get_skill_definition_by_local_id to consult this new map (lookup provider id
then return skill_definitions.get(...) or return the stored definition
directly).

In `@src/skills/catalog.v1.toml`:
- Around line 807-812: The provider_skill_id
"nodnarbnitram/claude-code-extensions/tauri-v2" does not match the
install_source repository "https://github.com/delexw/claude-code-misc...";
update the metadata so they align by either changing provider_skill_id to
"delexw/claude-code-misc/tauri-v2" (or the correct owner/path) to match
install_source, or add a clarifying comment/field documenting that the skill was
forked/moved and kept local_skill_id "tauri-v2" and title "Tauri v2"
intentionally point to the delexw repo; ensure provider_skill_id,
install_source, and any publishing metadata consistently reference the same
source.

In `@src/skills/suggest.rs`:
- Around line 331-334: annotate_recommendations currently checks installed
status by directly doing get(&recommendation.skill_id), which ignores
legacy_local_skill_ids; update annotate_recommendations to call
installed_state_for_recommendation(&installed_skill_states, recommendation) (the
same helper used by build_response) when computing the installed state and then
pass that into recommendation.annotate_installed_state(...), so renamed/aliased
skills are correctly detected for callers like recommend_skills ->
annotate_recommendations; ensure you reference the existing
installed_skill_states parameter and the installed_state_for_recommendation
helper rather than direct map lookups.

In `@tests/test_catalog_integration.rs`:
- Around line 45-62: The test helper resolve_install_source duplicates
resolution logic from SuggestInstallProvider::resolve; replace the duplicated
logic by reusing the production resolver or extracting the shared resolution
into a common function: either (A) call
SuggestInstallProvider::default().resolve(provider_skill_id) and return its
download_url (expose SuggestInstallProvider if necessary), or (B) move the
resolution logic that checks EmbeddedSkillCatalog::get_install_source,
DALLAY_SKILLS_PREFIX handling, and provider.resolve into a new shared function
(e.g., resolve_install_source_shared) used by both the test and
src/commands/skill.rs; update imports and signatures (local_skill_source_dir,
EmbeddedSkillCatalog, DALLAY_SKILLS_PREFIX) accordingly.
- Around line 64-76: install_with_retry currently discards the details of the
first failure if the retry succeeds; modify install_with_retry (which calls
blocking_fetch_and_install_skill) to log the first_error before sleeping and
retrying so the initial failure is available for debugging (use the project's
logging facility or eprintln!/warn! as appropriate), then keep the existing
retry behavior and existing combined anyhow::anyhow! on double failure.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 862450bb-487e-4152-803a-8ac56a6f1dd4

📥 Commits

Reviewing files that changed from the base of the PR and between f10842d and fffb85b.

📒 Files selected for processing (12)
  • .github/workflows/catalog-e2e.yml
  • README.md
  • src/commands/skill.rs
  • src/skills/catalog.rs
  • src/skills/catalog.v1.toml
  • src/skills/provider.rs
  • src/skills/suggest.rs
  • tests/integration/skill_suggest.rs
  • tests/test_catalog_integration.rs
  • tests/unit/provider.rs
  • tests/unit/suggest_catalog.rs
  • tests/unit/suggest_install.rs

Comment on lines +331 to +334
recommendation.annotate_installed_state(installed_state_for_recommendation(
&installed_skill_states,
recommendation,
));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Finish applying the alias-aware installed lookup to the public helper.

build_response now honors legacy_local_skill_ids, but annotate_recommendations(...) below still does a direct get(&recommendation.skill_id). Any caller that uses recommend_skills(...) followed by annotate_recommendations(...) will keep reporting renamed skills as not installed, so this feature is only correct through SuggestionService.

🔧 Suggested fix
 pub fn annotate_recommendations(
     recommendations: &mut [SkillSuggestion],
     installed_skill_states: &BTreeMap<String, InstalledSkillState>,
 ) {
     for recommendation in recommendations {
-        recommendation
-            .annotate_installed_state(installed_skill_states.get(&recommendation.skill_id));
+        recommendation.annotate_installed_state(installed_state_for_recommendation(
+            installed_skill_states,
+            recommendation,
+        ));
     }
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/skills/suggest.rs` around lines 331 - 334, annotate_recommendations
currently checks installed status by directly doing
get(&recommendation.skill_id), which ignores legacy_local_skill_ids; update
annotate_recommendations to call
installed_state_for_recommendation(&installed_skill_states, recommendation) (the
same helper used by build_response) when computing the installed state and then
pass that into recommendation.annotate_installed_state(...), so renamed/aliased
skills are correctly detected for callers like recommend_skills ->
annotate_recommendations; ensure you reference the existing
installed_skill_states parameter and the installed_state_for_recommendation
helper rather than direct map lookups.

@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Apr 6, 2026

Quality Gate Failed Quality Gate failed

Failed conditions
67.5% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@yacosta738 yacosta738 merged commit 97f5f71 into main Apr 6, 2026
24 of 26 checks passed
@yacosta738 yacosta738 deleted the fix/catalog-installability-audit branch April 6, 2026 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant